Conversation
… null-safe SerializerHelper registers a moshi EnumJsonAdapter with withUnknownFallback per enum, but EnumJsonAdapter is not null-safe and it is registered bare: any model with a nullable enum property throws "value was null! Wrap in .nullSafe() to write nullable values" on a null value, reading and writing alike. The flag traded unknown-value tolerance for a regression on every optional enum field. Append .nullSafe() in both branches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
|
Confirming the diagnosis: We hit this in production. It is worth spelling out where it fires, because it is not only a decode problem. The generated Serializer.moshi.adapter(T::class.java).toJson(content)That is the outbound path. Any request whose model has a null optional enum throws before it leaves the client. We reverted enabling the flag across our services because of it. I built and ran both sides rather than reading the diff. What I ran
Generated output between the two jars differs only in the four master this PR Checks 3 and 4 pass on both sides, which is the part I most wanted to see: The new unit test discriminates. It passes on this branch. Applied on top of master with the template left alone, it fails: A few things I checked while I was in there
One likely reason this went unnoticed for so long: If it is useful, the reproduction is on a branch: https://github.com/thejeff77/openapi-generator/tree/chore/verify-kotlin-enum-nullsafe-24894/enum-nullsafe-repro — a spec, a short Thanks for writing this up, @wiebren. The diagnosis and the fix both look right to me. Glad to help move it along — more cases, a runtime test in the samples, or a rebase onto current master, whichever is most useful. |
|
@wiebren — this has been sitting for two weeks with green CI and no triage, so here is what has actually worked for me on Kotlin PRs in this repo, in case it is useful. Tagging the technical committee mostly does not get a response. You did it correctly in the description, but the listed Kotlin members have reviewed almost nothing this year — counting reviews since January: What worked was Slack. On #23444 I tagged the committee, bumped after a week, then heard nothing for six weeks — until @wing328 commented asking me to resolve conflicts and then "PM me via Slack to have this reviewed and merged". It merged the next day. There is a public invite link at the top of the README. Two things that should make that conversation short:
For whatever it is worth as supporting evidence: I rebuilt both sides and ran the generated clients — details in my earlier comment. The fix is correct and complete, your test genuinely discriminates, and we reverted enabling this flag across our services because of the bug you are fixing. Happy to add a +1 wherever it helps. |
|
@thejeff77 thanks for rebuilding both sides and running the generated clients; that is more thorough than the PR's own test, and the outbound The missing |
enumUnknownDefaultCase=truedoes what it promises for unknown wire values in the moshikotlin client: every enum gains an
unknown_default_open_apimember andSerializerHelper.addEnumUnknownDefaultCaseregisters acom.squareup.moshi.adapters.EnumJsonAdapterwithwithUnknownFallbackper enum. ButEnumJsonAdapteris not null-safe, and the helper registers it bare — so any model witha nullable enum property now throws on a null value, reading and writing alike:
Concretely: serializing an update command whose optional enum field is unset, or
deserializing a response whose optional enum field is null. Found while running a generated
kotlin client (jvm-okhttp, moshi) against a production registry API on v7.15.0: enabling the
flag made unknown enum values tolerated and simultaneously broke every request/response with
an absent optional enum field. Without the flag, moshi's built-in enum handling is null-safe
and the same payloads round-trip fine — the flag is a strict regression for nullable enum
fields.
The fix
Append
.nullSafe()to each registered adapter injvm-common/infrastructure/SerializerHelper.kt.mustache, in both the top-level-enum andinline-enum branches.
withUnknownFallbackreturnsEnumJsonAdapter,.nullSafe()wraps itas a
JsonAdapter— which is whatMoshi.Builder.add(Type, JsonAdapter)takes — so thechange is two template lines.
Tests
KotlinClientCodegenModelTest#testMoshiEnumUnknownDefaultCaseAdaptersAreNullSafegeneratesfrom the existing
3_0/enum.yamlfixture with the flag on and asserts every registeredadapter is wrapped. Fails without the template change (verified by stashing only the
template).
PR checklist
./bin/generate-samples.sh ./bin/configs/kotlin*.yaml).One sample changes:
kotlin-enum-default-value, the only moshi config with the flag on —its five adapters (top-level and inline enums both) gain
.nullSafe().Generated with Claude Code
Summary by cubic
Enabling
enumUnknownDefaultCasein the Moshi Kotlin client no longer breaks nullable enum fields. Previously, the fallbackEnumJsonAdapterwas registered bare, so any null enum value threw aNullPointerExceptionon read and write; the adapters are now wrapped with.nullSafe()in both the top-level and inline enum branches.Tests
kotlin-enum-default-valuesample to reflect the new.nullSafe()calls.Written for commit 0cb5a0d. Summary will update on new commits.